Add MathProgBase solver interface. #9

tkoolen · 2018-02-02T01:02:44Z

I know MathOptInterface is coming soon, but I want to try this with JuMP right now.

No tests yet, but most things seem to be working. Demo code:

using OSQP, JuMP

solver = OSQPSolver()
MathProgBase.setparameters!(solver; Silent = true)
m = Model(solver = solver)
@variable(m, x)
@variable(m, y)
@constraint(m, 1 <= x <= 4)
@constraint(m, y >= 3)
@objective(m, Min, x^2 + y^2)
status = solve(m)
@show getvalue(x)
@show getvalue(y)
@show getobjectivevalue(m)

For ease of implementation, I decided to store copies of P, q, etc. in OSQPModel and call OSQP.setup! in MathProgBase.optimize!. None of the updating infrastructure is currently used. Still, it's a start.

mlubin · 2018-02-02T01:08:08Z

Looks reasonable. You should run the relevant functions in MathProgBase/test/quadprog.jl as part of the unit tests.

But I'd add that the important MOI infrastructure for QPs is now in place in JuMP, and we could use some early testers.

mlubin · 2018-02-02T01:24:24Z

src/OSQPSolverInterface.jl

+    settings::Dict{Symbol,Any}
+end
+
+OSQPSolver() = OSQPSolver(Dict{Symbol, Any}())


Maybe name this OSQPMPBSolver to avoid the imminent conflict with MOI?

I see this is still the common approach in most of the current MathProgBase interfaces, e.g., EcosSolverInterface.jl.

@mlubin Are you planning to change the other solvers as well or to just drop the MathProgBase interfaces altogether?

MPB interfaces will all be moved into separate packages when we start releasing the new MOI interfaces. It's up to developer discretion whether the MOI interface will sit in the same package as the direct solver wrapper or in a separate package.

Then it could be better to move this interface to a separate package called MathProgBaseOSQP.jl as done in MathProgBaseMosek.jl. In this way we could keep OSQPSolver in both MPB and MOI interfaces and avoid using both them at the same time. Is this the purpose?

It seems a bit silly to create a package just for 200 lines of code that are going to be deprecated soon. (I'm reconsidering if we should do this for all solvers.) If we remove the line:

import .OSQPSolverInterface: OSQPSolver

then there's nothing much to worry about with having both MPB and MOI interfaces in the same package.

Renamed the module to OSQPMathProgBaseInterface (solver is still named OSQPSolver). No longer exporting OSQPSolver, but exporting OSQPMathProgBaseInterface instead. What do you think?

bstellato · 2018-02-02T04:17:47Z

Thank you for this. I guess the next steps are to:

Complete the remaining functions that are compatible with OSQP
Implement the problem data updates properly (this gives OSQP the real speedups by the way). I think this can be done by
- keeping track of the problem data as an internal copy of P, q, A, l, u
- calling the osqp function updates whenever possible, e.g., when setobj!(m, c) is called we call OSQP.update!(model, q=c)
- having a boolean flag to define whether a new setup is required when optimize! is called. This is useful when funcitons like addvar! are called.
Add the quadprog tests as in Gurobi.jl

tkoolen · 2018-02-05T16:01:29Z

You should run the relevant functions in MathProgBase/test/quadprog.jl as part of the unit tests.

Since OSQP doesn't support quadratic constraints, neither quadprogtest nor qpdualtest is really applicable. There's also the additional issue of lack of support for variable bounds (do you agree with my handling of that by the way, or should they automatically be turned into constraints?). I opted to just copy over the relevant parts of quadprogtest for now. The dual-related functionality should still be tested somehow though.

codecov-io · 2018-02-05T16:11:14Z

Codecov Report

Merging #9 into master will increase coverage by 8.22%.
The diff coverage is 89.35%.

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
+ Coverage   71.69%   79.91%   +8.22%     
==========================================
  Files           3        4       +1     
  Lines         272      488     +216     
==========================================
+ Hits          195      390     +195     
- Misses         77       98      +21

Impacted Files	Coverage Δ
src/OSQP.jl	`83.33% <ø> (ø)`	⬆️
src/interface.jl	`69.54% <ø> (+0.82%)`	⬆️
src/mpbinterface.jl	`89.35% <89.35%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb1ea2d...5094552. Read the comment docs.

mlubin · 2018-02-05T16:25:15Z

test/mpbinterface.jl

+    sol = quadprog([0., 0., 0.],[2. 1. 0.; 1. 2. 1.; 0. 1. 2.],[1. 2. 3.; 1. 1. 0.],'>',[4., 1.],-Inf,Inf,solver)
+    @test sol.status == :Optimal
+    @test isapprox(sol.objval, 130/70, atol=1e-6)
+    @test isapprox(norm(sol.sol[1:3] - [0.5714285714285715,0.4285714285714285,0.8571428571428572]), 0.0, atol=1e-6)


I'm surprised the test passes with these tolerances.

Oops, missed the setting for eps_abs above.

Actually, polish = true is more essential. Just setting eps_abs to even 1e-8 results in test failure.

Polish shouldn't be necessary, but you may need to set eps_rel (osqp/osqp#40) to a tiny value.

If you take the relative tolerance into account it might work without polishing:

@test isapprox(norm(sol.sol[1:3] - [0.5714285714285715,0.4285714285714285,0.8571428571428572]), rtol=1e-6, atol=1e-6)

mlubin · 2018-02-05T16:26:29Z

REQUIRE

@@ -1,3 +1,4 @@
 BinDeps
 julia 0.6
 Compat
+MathProgBase 0.7


It's always a good idea to put an upper bound on the MPB requirement.

mlubin · 2018-02-05T16:44:46Z

There's also the additional issue of lack of support for variable bounds (do you agree with my handling of that by the way, or should they automatically be turned into constraints?).

Depends on whether you want to call OSQP on JuMP models that have variables with bounds.

bstellato · 2018-02-05T16:47:15Z

Since OSQP doesn't support quadratic constraints, neither quadprogtest nor qpdualtest is really applicable. There's also the additional issue of lack of support for variable bounds (do you agree with my handling of that by the way, or should they automatically be turned into constraints?). I opted to just copy over the relevant parts of quadprogtest for now. The dual-related functionality should still be tested somehow though.

I think people use variable bounds a lot. We could keep track of them in the problem data as

    lb <= Ax <= ub
    l <= x <= u

Then, when we call optimize! we stack them together. I can add it later today or tomorrow.

Only allow :Min as the sense for now, as the current implementation can lead to confusing/erroneous behavior. Add freemodel!, copy, getsense, setsense!.

tkoolen · 2018-02-05T17:15:56Z

Re: #9 (comment): I started doing that initially, but I got a little worried that the interpretation of the dual variables would become confusing/nonstandard.

We also need better handling of sense. For now, I've restricted sense to be :Min in my latest commit in order to at least avoid blatantly wrong behavior. I'm not sure what the JuMP/MathProgBase convention is for handling duals when sense == :Max, so guidance would be appreciated. Also, the fact that loadproblem! contains the sense but not the 'P' matrix would have made subsequent calls to setquadobj! do the wrong thing for sense == :Max.

mlubin · 2018-02-05T17:55:11Z

For now, I've restricted sense to be :Min in my latest commit in order to at least avoid blatantly wrong behavior. I'm not sure what the JuMP/MathProgBase convention is for handling duals when sense == :Max, so guidance would be appreciated.

JuMP has lots of tests for duals on LPs with both min/max sense. Not sure if there are specific tests for QPs. You should really feel free to only implement the parts that you need, because all of this is going to be tossed out after JuMP 0.18. Rejecting Max problems is reasonable.

… interface

tkoolen · 2018-02-06T21:36:39Z

Improved test coverage. Also discovered and fixed a minor problem with OSQP.warm_start! along the way.

tkoolen · 2018-02-06T22:04:32Z

Don't merge yet; I suspect there's something wrong with the quadratic objective methods.

bstellato · 2018-02-06T22:46:31Z

Thanks for adding these tests. Tomorrow I will merge my changes as well to complete the interface.

tkoolen · 2018-02-07T18:46:33Z

Fixed the objective bug and added yet more tests that displayed the problem. I'm successfully using OSQP through JuMP for my intended application now.

…d to fix objective sense. Not tested yet

bstellato · 2018-02-08T14:49:49Z

src/mpbinterface.jl

+    end
+end
+
+function Base.resize!(model::OSQPModel, n, m)


Is the resize! function really necessary? it seems that it does not properly resize the problem but it just creates zero matrices P and A and resizes the vectors. I guess this is needed in the loadproblem! function where you call copy!. Is this necessary/more efficient than just using copy or deepcopy for the data?

FTR: answered on gitter. Yes, efficiency.

tkoolen

In general, there will be quite a lot of allocations when using the additional functionality. You could add some fields to the model to avoid those allocations. Or (and that's absolutely fine) don't bother and we'll handle it properly in the MathOptInterface version.

tkoolen · 2018-02-09T22:04:12Z

test/runtests.jl

+    # "primal_infeasibility.jl",
+    # "unconstrained.jl",
+    # "warm_start.jl",
+    # "update_matrices.jl",


tkoolen · 2018-02-09T22:05:07Z

src/mpbinterface.jl

+module OSQPMathProgBaseInterface
+
+using OSQP: Model, Results, setup!, solve!, update!, clean!, update_settings!, warm_start!
+importall MathProgBase.SolverInterface


importall is going away soon. See https://github.com/JuliaLang/julia/blob/master/NEWS.md#language-changes.

tkoolen · 2018-02-09T22:07:21Z

src/mpbinterface.jl

+    end
+end
+
+function get_qp_variables(model::OSQPMathProgModel)


Why not just have this be numvar?

tkoolen · 2018-02-09T22:07:55Z

src/mpbinterface.jl

+    return n
+end
+
+get_qp_constraints(model::OSQPMathProgModel) = size(model.A, 1)


numconstr?

tkoolen · 2018-02-09T22:09:00Z

src/mpbinterface.jl

+checksolved(model::OSQPMathProgModel) = isdefined(model, :results) || error("Model has not been solved.")
+
+# Reset problem when setup has to be performed again
+resetproblem(model::OSQPMathProgModel) = (model.results = nothing; model.perform_setup=true)


This shouldn't work. model.results can't store nothing.

tkoolen · 2018-02-09T22:15:44Z

src/mpbinterface.jl

+
+    # Negate cost if necessary
+    if model.sense == :Max
+        model.q *= -1


Allocation could be avoided, but not a huge deal.

tkoolen · 2018-02-09T22:19:57Z

src/mpbinterface.jl

+getconstrmatrix(model::OSQPMathProgModel) = model.A
+
+
+function addvar!(model::OSQPMathProgModel, constridx, constrcoef, l, u, objcoef)


Lots of allocations, to the point that it may not be beneficial to support this. I don't think JuMP even uses this.

tkoolen · 2018-02-09T22:21:37Z

src/mpbinterface.jl

+    @boundscheck length(colidx) == nterms || error()
+
+    # Check if only the values have changed
+    if isdefined(model, :P)


This is always true.

tkoolen · 2018-02-09T22:26:14Z

src/mpbinterface.jl

+    # Check if only the values have changed
+    if isdefined(model, :P)
+        Pi, Pj, Px = findnz(model.P)
+        if (rowidx == Pi) & (colidx == Pj) & !model.perform_setup


Given that ordering of rowidx and colidx shouldn't matter and

Duplicate index sets (i,j) are accepted and will be summed together

this is kind of a crude check.

I have left it for now since I cannot find an easier way to check if the sparsity pattern of P stays the same. If you have any suggestions feel free to update it.

tkoolen · 2018-02-09T22:28:40Z

src/mpbinterface.jl

+
+    # Change sign if maximizing
+    if model.sense == :Max
+        model.P *= -1


Expensive way of doing this.

bstellato · 2018-02-09T22:40:50Z

Damn, I pushed before finishing some stuff!!! Sorry. I had to rush out to finish some other things. I wanted to rebase everything before pushing it. I will adjust those things tomorrow ok? (Have lots of IKEA stuff to put together...).

Thanks anyway for the performance tips. I am not totally familiar with Julia performance guidelines yet. I haven't used it that much until very recently.

bstellato · 2018-02-12T19:50:46Z

I have fixed the interface. It should now support incremental model building, infeasibility certificates, Max or Min problems and problem data updates. There was a problem with the dual variables sign before. I have removed a couple of unnecessary allocations but I am quite sure we could do something better memory-wise. However, we might just leave it as it is and focus on performance improvements for the MathOptInterface. Let me know what you think.

tkoolen · 2018-02-13T00:12:08Z

Looks good to me!

Prep for Vector constraints.

tkoolen added 2 commits February 1, 2018 19:13

Add MathProgBase solver interface.

0d9ac6c

Add MathProgBase solver interface.

4f14108

mlubin reviewed Feb 2, 2018

View reviewed changes

Address review comments.

c27d244

tkoolen force-pushed the tk/mathprogbase branch from 0d9ac6c to c27d244 Compare February 5, 2018 15:46

Copy over relevant parts of MathProgBase test/quadprog.jl.

df2f7e0

mlubin reviewed Feb 5, 2018

View reviewed changes

Refine/implement more of MathProgBase interface.

9b061b6

Only allow :Min as the sense for now, as the current implementation can lead to confusing/erroneous behavior. Add freemodel!, copy, getsense, setsense!.

Upper bound on MPB.

de19e17

bstellato and others added 4 commits February 5, 2018 22:58

Added mathprog tests. Basic restructuring. Still need to complete the…

1510ae3

… interface

Merged both changes but still need to finish the interface

bce0012

Deleted solver interface in mathprog old file

4311dfd

Improve test coverage.

ff8f248

tkoolen force-pushed the tk/mathprogbase branch from d849131 to ff8f248 Compare February 6, 2018 21:36

Add more tests, fix objective bug.

938a50b

bstellato added 2 commits February 7, 2018 16:37

Merged tkoolen changes

683a6e5

Added changes for proper updates without performing setup again. Trie…

381c761

…d to fix objective sense. Not tested yet

Added update_settings! to setparameters

14e0d97

bstellato reviewed Feb 8, 2018

View reviewed changes

bstellato added 3 commits February 8, 2018 19:20

First tests working but still getting segfaults when I close julia

f4b4af9

Apparently setwarmstart! causes troubles

7a46756

Got Twan tests working

23f3202

tkoolen mentioned this pull request Feb 9, 2018

WIP: Implement MathOptInterface #11

Closed

22 tasks

Got more tests working. Need to fix last ones in linproginterface

ec0ab49

tkoolen commented Feb 9, 2018

View reviewed changes

bstellato added 4 commits February 11, 2018 23:12

Fixed some bugs

ed9b220

Got all mathprogbase tests to work. Fixed dual variables signs

1299cc5

Other adjustments based on comments

d11c474

Fixed setquadobj! to reset problem status forcing a new solve

5094552

Edited changelog before merging

47f7010

bstellato merged commit 69d25e7 into osqp:master Feb 13, 2018

tkoolen deleted the tk/mathprogbase branch February 13, 2018 03:16

bstellato pushed a commit that referenced this pull request May 17, 2018

Merge pull request #9 from tkoolen/tk/vector-constraints-prep

ac2359b

Prep for Vector constraints.

		getconstrmatrix(model::OSQPMathProgModel) = model.A


		function addvar!(model::OSQPMathProgModel, constridx, constrcoef, l, u, objcoef)

Add MathProgBase solver interface. #9

Add MathProgBase solver interface. #9

Conversation

tkoolen commented Feb 2, 2018

mlubin commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstellato commented Feb 2, 2018

tkoolen commented Feb 5, 2018

codecov-io commented Feb 5, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlubin commented Feb 5, 2018

bstellato commented Feb 5, 2018

tkoolen commented Feb 5, 2018 • edited Loading

mlubin commented Feb 5, 2018

tkoolen commented Feb 6, 2018

tkoolen commented Feb 6, 2018

bstellato commented Feb 6, 2018

tkoolen commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkoolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstellato commented Feb 9, 2018

bstellato commented Feb 12, 2018

tkoolen commented Feb 13, 2018

codecov-io commented Feb 5, 2018 •

edited

Loading

tkoolen commented Feb 5, 2018 •

edited

Loading